Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for STI scoping across leaves #16775

Merged
merged 1 commit into from
Jan 15, 2018
Merged

Fix for STI scoping across leaves #16775

merged 1 commit into from
Jan 15, 2018

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Jan 9, 2018

This addresses a concern that classes using STI do not work with the unique_within_region validation (that currently uses record.class). The validation may not properly scope across leaf classes. The suggestion was to use record.class.base_class instead.

😭

per #16739

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 9, 2018

@miq-bot assign @gmcculloug

@miq-bot miq-bot added the wip label Jan 9, 2018
@d-m-u d-m-u changed the title [WIP] Fix for STI scoping across leafs Fix for STI scoping across leaves Jan 9, 2018
@miq-bot miq-bot removed the wip label Jan 9, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 10, 2018

@Fryguy I think this's what you wanted.

@carbonin
Copy link
Member

@d-m-u I think you could probably DRY up all the record.class.base_class here.

@d-m-u d-m-u changed the title Fix for STI scoping across leaves [WIP] Fix for STI scoping across leaves Jan 10, 2018
@miq-bot miq-bot added the wip label Jan 10, 2018
@d-m-u d-m-u force-pushed the issue_16739 branch 2 times, most recently from c0f4a9e to e948ab2 Compare January 10, 2018 18:31
@d-m-u d-m-u changed the title [WIP] Fix for STI scoping across leaves Fix for STI scoping across leaves Jan 10, 2018
@miq-bot miq-bot removed the wip label Jan 10, 2018
@miq-bot
Copy link
Member

miq-bot commented Jan 10, 2018

This pull request is not mergeable. Please rebase and repush.

Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, 👍 LGTM

But... it looks like something else happened with a merge commit. Maybe a bad rebase?
image

@miq-bot
Copy link
Member

miq-bot commented Jan 13, 2018

This pull request is not mergeable. Please rebase and repush.

@gmcculloug
Copy link
Member

@d-m-u Please rebase

@miq-bot
Copy link
Member

miq-bot commented Jan 15, 2018

Checked commit d-m-u@ad0f581 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 👍

@bdunne bdunne merged commit c992bac into ManageIQ:master Jan 15, 2018
@bdunne bdunne added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 15, 2018
@bdunne bdunne assigned bdunne and unassigned gmcculloug Jan 15, 2018
@d-m-u d-m-u deleted the issue_16739 branch January 29, 2018 13:16
@jrafanie
Copy link
Member

Adding gaprindashvili/yes since this is needed to backport #17544 to gaprindashvili

simaishi pushed a commit that referenced this pull request Jun 14, 2018
Fix for STI scoping across leaves
(cherry picked from commit c992bac)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit b2a95afaa7dcd80abe22aa0b488e4d3eced8fc11
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Mon Jan 15 15:41:47 2018 -0500

    Merge pull request #16775 from d-m-u/issue_16739
    
    Fix for STI scoping across leaves
    (cherry picked from commit c992bac4008916e65ebf922f150af79ba4a85a1a)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants